west manifest: detect when target directory already exists, and fail #676
Conversation
There was a problem hiding this comment.
This looks like a good catch, thank you! Just a bit light on the English part of it :-) Can you please:
-
Flesh out the commit message with a short description of how to reproduce the issue that this commit fixes. It's great that you reference west init completes but leaves everything in zephyr/manifest-tmp #558 but 558 has turned into a long and unreadable mess of multiple and possibly unrelated issues as recently noted there by someone named... "attie-argumentum" :-) Quoting: "I believe that the issue reported by X / Y / Z is different to the original issue". For now the commit message describes only what the fix does NOT do!
-
Please add a one-line comment in the code, here's a suggestion below. Also, move the check between the
mkdirandmovecommands which are closed related: them being close helps understand the issue too.
PS: next time please add some reviewers or "tag" some people.
--- a/src/west/app/project.py
+++ b/src/west/app/project.py
@@ -340,6 +340,10 @@ With neither, -m {MANIFEST_URL_DEFAULT} is assumed.
self.dbg('moving', tempdir, 'to', manifest_abspath,
level=Verbosity.DBG_EXTREME)
manifest_abspath.parent.mkdir(parents=True, exist_ok=True)
+ # What happens in that case is OS and filesystem-dependent; we
+ # _really_ don't want to go there.
+ if manifest_abspath.exists():
+ self.die(f'target directory already exists ({manifest_abspath})')
try:
shutil.move(os.fspath(tempdir), os.fspath(manifest_abspath))
except shutil.Error as e:
|
Thanks and sorry for the long review time. I agree that it's not clear that this PR fixes #558 but that it indeed looks like a good fix. |
|
Also, the |
attie-argentum
force-pushed
the
manifest-tmp
branch
2 times, most recently
from
225cfa7
to
d8f2e5e
Compare
|
Very fair comments - thanks both. Apologies for not tagging anyone, I honestly submitted this and forgot about it. 🤦♀️ I've made changes that hopefully address your concerns.
From my reading of #558, I'd agree with that path forwards - locking on Windows, due to other tools.
Wow, I've seen some stuff spill over from forks elsewhere, but that's not good! Removed. |
When setting up a project with west, the target directory may not be initialized correctly. In the typical case, if a directory named `./zephyr/` already exists, the user may find that checkout files are located at `./zephyr/manifest-tmp/*` instead of the expected `./zephyr/*`. This patch will abort and refuse to complete `west init` if the destination directory alread exists. This check would ideally occur before the potentially lengthy clone operation, but `manifest_path` is derived from the files retrieved... NOTE: If the project quotes a value other than `zephyr` for `manifest.self.path` in `/west.yml`, then this will affect that directory instead. Steps to reproduce before this patch: mkdir ./zephyr/ west init ./ -m https://github.com/zephyrproject-rtos/zephyr.git ls -l ./zephyr/ Possible fix for some of the symptoms described in zephyrproject-rtos#558 Signed-off-by: Attie Grande <[email protected]>
attie-argentum
force-pushed
the
manifest-tmp
branch
from
d8f2e5e
to
7b7fa78
Compare
|
No problem - I've made both of these changes too. |
|
Proof that this new error message is useful: #684 |
This detection would ideally occur before the lengthy clone operation, but
manifest_pathis derived from the files retrieved...EDIT: see newer commit message